Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix linked objctive-C symbols on old macOS versions #46

Merged
merged 2 commits into from
Aug 6, 2022
Merged

Fix linked objctive-C symbols on old macOS versions #46

merged 2 commits into from
Aug 6, 2022

Conversation

BlackHoleFox
Copy link
Contributor

Closes #45

With these changes, the symbol table for binaries using this crate looks more correct for older macOS builds. By default on a M1:

~/x/leak_hunting [master +4 ~0 -0 !] ▶ nm ./target/debug/leak_hunting | rg _OBJC_CLASS_
                 U _OBJC_CLASS_$_NSAppleScript
                 U _OBJC_CLASS_$_NSBundle
                 U _OBJC_CLASS_$_NSConstantDictionary
                 U _OBJC_CLASS_$_NSConstantIntegerNumber
                 U _OBJC_CLASS_$_NSDate
                 U _OBJC_CLASS_$_NSDictionary
                 U _OBJC_CLASS_$_NSImage
                 U _OBJC_CLASS_$_NSObject
                 U _OBJC_CLASS_$_NSRunLoop
                 U _OBJC_CLASS_$_NSString
                 U _OBJC_CLASS_$_NSThread
                 U _OBJC_CLASS_$_NSURL
                 U _OBJC_CLASS_$_NSUserNotification
                 U _OBJC_CLASS_$_NSUserNotificationCenter
0000000100068748 S _OBJC_CLASS_$_NotificationCenterDelegate
0000000100068398 s __OBJC_CLASS_PROTOCOLS_$_NotificationCenterDelegate
00000001000684a8 s __OBJC_CLASS_RO_$_NotificationCenterDelegate

With MACOSX_DEPLOYMENT_TARGET="10.14" set:

~/x/leak_hunting [master +4 ~0 -0 !] ▶ nm ./target/debug/leak_hunting | rg _OBJC_CLASS_
                 U _OBJC_CLASS_$_NSAppleScript
                 U _OBJC_CLASS_$_NSBundle
                 U _OBJC_CLASS_$_NSDate
                 U _OBJC_CLASS_$_NSDictionary
                 U _OBJC_CLASS_$_NSImage
                 U _OBJC_CLASS_$_NSNumber
                 U _OBJC_CLASS_$_NSObject
                 U _OBJC_CLASS_$_NSRunLoop
                 U _OBJC_CLASS_$_NSString
                 U _OBJC_CLASS_$_NSThread
                 U _OBJC_CLASS_$_NSURL
                 U _OBJC_CLASS_$_NSUserNotification
                 U _OBJC_CLASS_$_NSUserNotificationCenter
0000000100068a38 S _OBJC_CLASS_$_NotificationCenterDelegate
0000000100068668 s __OBJC_CLASS_PROTOCOLS_$_NotificationCenterDelegate
0000000100068778 s __OBJC_CLASS_RO_$_NotificationCenterDelegate

Additionally, and probably importantly, the dictionary symbols better match what XCode produces with the relevant target versions. @betamos can you try this with Tauri on 10.14/10.13?

@betamos
Copy link
Contributor

betamos commented Jul 17, 2022

Confirming! I built a universal binary with Tauri using minimumSystemVersion: 10.13 in my tauri.conf.json.

Tauri build (tauri-cli 1.0.4) apparently does not set the MACOSX_DEPLOYMENT_TARGET env var flag (it does set it for rustc, but it doesn't propagate to the build.rs in mac-notification-sys). I consider this a bug on the Tauri side and will file an issue there - don't worry about that atm.

Anyway, due to the way you set the default to 10.8, it worked anyway. 🎉

More hairy details about what happened:

  • I'm building a universal binary which combines aarch64 and x86_64 binaries under the hood in tauri build.
  • The NSConstantDictionary was present in the aarch64 but not in the x86_64 build (again, expected due to your defaults).
  • As such, the symbol was present in the combined universal build.
  • ...But this was not a problem, due to the way universal binaries work. The resulting app/.dmg did launch without the dyld error.

LGTM!

betamos
betamos reviewed Jul 17, 2022
View reviewed changes
build.rs Show resolved Hide resolved
@betamos betamos mentioned this pull request Jul 17, 2022
Closed
@betamos
Copy link
Contributor

betamos commented Jul 21, 2022

Just a friendly reminder: please consider getting this in and pushing a new minor release. Mac compatibility is broken atm :(

Thank you!

@lucasfernog
Copy link
Contributor

I would love to see this patch getting merged too!

@hoodie
Copy link
Collaborator

hoodie commented Aug 6, 2022

thank you!

@hoodie hoodie merged commit 1944fb3 into h4llow3En:master Aug 6, 2022
@hoodie
Copy link
Collaborator

hoodie commented Aug 6, 2022

habemus v0.5.6

@BlackHoleFox BlackHoleFox deleted the maybe-symbol-fixes branch August 6, 2022 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@betamos betamos betamos left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Possible version mismatch in CoreFoundation.framework
4 participants
@BlackHoleFox @betamos @lucasfernog @hoodie